-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rviz crash issue if run later #4871
Conversation
@mini-1235, all pull requests must be targeted towards the |
Should I close this and re-open one targeting branch main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please close this PR on Jazzy and apply to main
, I'll backport the main PR to jazzy once complete and reviews are accepted :-)
Please also look at the DCO / linting jobs that failed, those will need to be fixed before a PR can be merged.
Thanks for taking a look at this, this looks like a good method to me!
client_node_, "smoother_server", "smoother_plugins", smoother_); | ||
nav2_rviz_plugins::pluginLoader( | ||
client_node_, "controller_server", "progress_checker_plugins", | ||
progress_checker_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to track and log the failures here so that we can print something that it failed to connect. I think the server_failed_
should be maintained
const std::string & plugin_type, QComboBox * combo_box) | ||
{ | ||
auto parameter_client = std::make_shared<rclcpp::SyncParametersClient>(node, server_name); | ||
|
||
// Do not load the plugins if the combo box is already populated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
while (!parameter_client->wait_for_service(std::chrono::seconds(1))) { | ||
if (!rclcpp::ok()) { | ||
RCLCPP_ERROR(node->get_logger(), "Interrupted while waiting for the service. Exiting."); | ||
rclcpp::shutdown(); | ||
} | ||
RCLCPP_INFO(node->get_logger(), "%s service not available", server_name.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove? If this failed, shouldn't we log and notify that the server is unavailable to not try to call the client below it?
I tseems like the server_unavailable should be maintained.
Thanks for the suggestions! I will give another fix targeting the main branch and open a new PR after I have finished it |
Basic Info
Description of contribution in a few bullet points
On my machine, this not only stuck in getting parameter of planner, but also other plugins like controller, goal checker and so on. Thus, I did the following to solve this issue:
get_parameter()
would not block the main threadDescription of documentation updates required from your changes
None
Description of how this change was tested
Extensively tested on tb3 simulation
Future work that may be required in bullet points
On the original implementation, the combo box will appear blank if it cannot get parameter from service, I now put it a "default" on those labels, I think this will be better (otherwise users will click on an empty label).
I am not really familiar with QT, not sure if there is any problem in my implementation :)
Currently only tested on Jazzy, but I could build new images and test it on different distributions
For Maintainers: